Convert manifests + BSL api client to kubebuilder #2561
Conversation
carlisia
changed the title
carlisia
force-pushed
the
c-kb
branch
3 times, most recently
from
b25c7b6 to
d8b786d
Compare
carlisia
requested review from
skriss,
nrb and
ashish-amarnath
and removed request for
skriss
|
Rebasing this PR is not fun @vmware-tanzu/velero-maintainers! |
carlisia
force-pushed
the
c-kb
branch
4 times, most recently
from
5880d10 to
d2ce999
Compare
|
Moved the BSL api into the old package. Cleaned up makefile and the update crd code script. Deleting the entire Pending: addressing questions and maybe a couple clean ups as per the reviews so far, but no big change expected. |
|
@vmware-tanzu/velero-maintainers PTAL when you have a chance, it's ready for review. There are 3 //todo(carlisia) to call attention to some questions I have about how to best refactor those code sections. All other reviews were addressed, minus one question for Ashish (see above). |
Signed-off-by: Carlisia <carlisia@vmware.com>
This will allow for proper cache management. Signed-off-by: Carlisia <carlisia@vmware.com>
Signed-off-by: Carlisia <carlisia@vmware.com>
Signed-off-by: Carlisia <carlisia@vmware.com>
Signed-off-by: Carlisia <carlisia@vmware.com>
Signed-off-by: Carlisia <carlisia@vmware.com>
Signed-off-by: Carlisia <carlisia@vmware.com>
Signed-off-by: Carlisia <carlisia@vmware.com>
Signed-off-by: Carlisia <carlisia@vmware.com>
There was a problem hiding this comment.
Thanks for your hard work in getting this done Carlisia! Definitely appreciate your patience and willingness to tackle this technical debt!
@ashish-amarnath If you're comfortable with this code, let's squash it rather than doing a plain merge.
nrb
dismissed
skriss’s stale review
Steve's comments have since been addressed, he just hasn't come back to the PR.
| cmd.CheckError(err) | ||
|
|
||
| var locations *api.BackupStorageLocationList | ||
| locations := new(velerov1api.BackupStorageLocationList) |
There was a problem hiding this comment.
quick question, I wonder if there is a specific reason for using the new keyword rather than velerov1api.BackupStorageLocationList{}?
The latter seems to be used couple lines later and it would be good to have the same pattern.
There was a problem hiding this comment.
Here we want to create a new list object and not a slice of backupstoragelocations.
This, IMO, is correct.
| go func() { | ||
| select { | ||
| case <-parentCh: | ||
| cancel() |
There was a problem hiding this comment.
wonder if this go func should return here?
There was a problem hiding this comment.
Not sure if you mean instead of cancel() otherwise there's no need to add a return, that's the default behavior.
There was a problem hiding this comment.
I mean, it returns from the switch but then falls down to return ctx, cancel in the end. I don't think it's needed to add that here, this seems fine to me. If I misunderstood please let me know.
There was a problem hiding this comment.
There's no need for an explicit return - if this case is hit it will cancel the child context, exit the select statement, and then exit the goroutine.
| select { | ||
| case <-parentCh: | ||
| cancel() | ||
| case <-ctx.Done(): |
There was a problem hiding this comment.
out of curiosity, if this case is a no-op, would it make sense for it to be removed?
There was a problem hiding this comment.
How is this case a no-op? The goroutine's mission is to either close the child context if the parent is closed, or exit when only the child is cancelled. The child can be cancelled without the parent being closed. Without the second case - the goroutine would leak.
after that!Signed-off-by: Carlisia <carlisia@vmware.com>
Signed-off-by: Carlisia <carlisia@vmware.com>
Signed-off-by: Carlisia <carlisia@vmware.com>
Closes: #2604
TODO:
https://www.loom.com/share/b96ce9e9cd3846428ef4243ed9a1828a